[llvm-readobj][offload] Fix llvm-readobj --all on MachO#175912
[llvm-readobj][offload] Fix llvm-readobj --all on MachO#175912
Conversation
Currently running llvm-readobj --all on any MachO object asserts, because it implies --offload, and the --offload extraction includes an assert on the object format. Instead, we should be silently ignoring.
|
@llvm/pr-subscribers-llvm-binary-utilities Author: Nikita Popov (nikic) ChangesCurrently running llvm-readobj --all on any MachO object asserts, because it implies --offload, and the --offload extraction includes an assert on the object format. Instead, we should be silently ignoring. This regressed in #143342. Full diff: https://github.com/llvm/llvm-project/pull/175912.diff 2 Files Affected:
diff --git a/llvm/lib/Object/OffloadBundle.cpp b/llvm/lib/Object/OffloadBundle.cpp
index 046cde8640b49..f006298c1f4b9 100644
--- a/llvm/lib/Object/OffloadBundle.cpp
+++ b/llvm/lib/Object/OffloadBundle.cpp
@@ -188,7 +188,9 @@ Error OffloadBundleFatBin::extractBundle(const ObjectFile &Source) {
Error object::extractOffloadBundleFatBinary(
const ObjectFile &Obj, SmallVectorImpl<OffloadBundleFatBin> &Bundles) {
- assert((Obj.isELF() || Obj.isCOFF()) && "Invalid file type");
+ // Ignore unsupported object formats.
+ if (!Obj.isELF() && !Obj.isCOFF())
+ return Error::success();
// Iterate through Sections until we find an offload_bundle section.
for (SectionRef Sec : Obj.sections()) {
diff --git a/llvm/test/tools/llvm-readobj/MachO/all.test b/llvm/test/tools/llvm-readobj/MachO/all.test
new file mode 100644
index 0000000000000..8916d72614cc5
--- /dev/null
+++ b/llvm/test/tools/llvm-readobj/MachO/all.test
@@ -0,0 +1,34 @@
+# RUN: yaml2obj %s | llvm-readobj --all - | FileCheck %s
+
+# Make sure that --all at least doesn't crash.
+
+# CHECK: Format: Mach-O arm
+# CHECK: Arch: arm
+# CHECK: AddressSize: 32bit
+# CHECK: MachHeader {
+# CHECK: Magic: Magic (0xFEEDFACE)
+# CHECK: CpuType: Arm (0xC)
+# CHECK: CpuSubType: CPU_SUBTYPE_ARM_V7 (0x9)
+# CHECK: FileType: Relocatable (0x1)
+# CHECK: NumOfLoadCommands: 0
+# CHECK: SizeOfLoadCommands: 0
+# CHECK: Flags [ (0x0)
+# CHECK: ]
+# CHECK: }
+# CHECK: Sections [
+# CHECK: ]
+# CHECK: Relocations [
+# CHECK: ]
+# CHECK: UnwindInfo not implemented.
+# CHECK: Symbols [
+# CHECK: ]
+
+--- !mach-o
+FileHeader:
+ magic: 0xFEEDFACE
+ cputype: 0xC
+ cpusubtype: 0x9
+ filetype: 0x1
+ ncmds: 0
+ sizeofcmds: 0
+ flags: 0x0
|
jh7370
left a comment
There was a problem hiding this comment.
A part of me is wondering if this is the right place for the fix, but I'd actually prefer @david-salinas to make that decision. I could see a case that the --all option should only enable Offloading for the file formats that support it (either explicitly or via the ObjDumper class hierarchy in llvm-readobj's code).
Not directly related to this PR, but one thing is for certain: the option shouldn't be set by --all for GNU style output (i.e. llvm-readelf), because GNU readelf doesn't have the option. This is in keeping with things like Addrsig.
| @@ -0,0 +1,34 @@ | |||
| # RUN: yaml2obj %s | llvm-readobj --all - | FileCheck %s | |||
|
|
|||
| # Make sure that --all at least doesn't crash. | |||
There was a problem hiding this comment.
Nit: newer llvm-readobj tests use ## for comments to help them stand out from lit/FileCheck directives.
Even if it's not part of |
|
True, in which case it should be either the fix you've got here or the class hierarchy rework (where offloading is only implemented for the supported platforms). As I don't maintain the offloading API, I think it's worth getting someone who does to make the decision. |
|
I'm torn on whether or not |
jhuber6
left a comment
There was a problem hiding this comment.
I'm fine with this fix, but if people's would rather omit the offloading flag from this mode I'd be fine there as well.
jh7370
left a comment
There was a problem hiding this comment.
I'm fine with this fix, but if people's would rather omit the offloading flag from this mode I'd be fine there as well.
I think removing it from the --all mode for GNU output is the right thing to do (could you or @david-salinas or someone else with offloading ownership make that change, please?), but as @nikic has already highlighted, that's not enough to fix the issue, so if you're happy with this change, I am too. LGTM.
|
/cherry-pick 7abe5b7 |
|
/pull-request #176133 |
Currently running llvm-readobj --all on any MachO object asserts, because it implies --offload, and the --offload extraction includes an assert on the object format. Instead, we should be silently ignoring. This regressed in llvm#143342. (cherry picked from commit 7abe5b7)
Currently running llvm-readobj --all on any MachO object asserts, because it implies --offload, and the --offload extraction includes an assert on the object format. Instead, we should be silently ignoring. This regressed in llvm#143342.
Currently running llvm-readobj --all on any MachO object asserts, because it implies --offload, and the --offload extraction includes an assert on the object format. Instead, we should be silently ignoring. This regressed in llvm#143342.
Update to LLVM 22 Scheduled release date: Feb 24 1.94 becomes stable: Mar 5 Changes: * Update to rc2, with one patch to work around our outdated illumos sysroot (rust-lang/llvm-project@41256ab). * Update the host toolchain as well, otherwise we lose cross-language LTO, in particular for jemalloc. * Adjust one loongarch assembly test. The split into r and s variants is based on the suggestion in #151134. Depends on: * [x] #151410 * [ ] #150756 * [x] llvm/llvm-project#175190 * [x] llvm/llvm-project#175912 * [x] llvm/llvm-project#175965 * [x] llvm/llvm-project#176195 * [x] llvm/llvm-project#157073 * [x] llvm/llvm-project#176421 * [x] llvm/llvm-project#176925 * [x] llvm/llvm-project#177187
Update to LLVM 22 Scheduled release date: Feb 24 1.94 becomes stable: Mar 5 Changes: * Update to rc2, with one patch to work around our outdated illumos sysroot (rust-lang/llvm-project@41256ab). * Update the host toolchain as well, otherwise we lose cross-language LTO, in particular for jemalloc. * Adjust one loongarch assembly test. The split into r and s variants is based on the suggestion in #151134. Depends on: * [x] #151410 * [ ] #150756 * [x] llvm/llvm-project#175190 * [x] llvm/llvm-project#175912 * [x] llvm/llvm-project#175965 * [x] llvm/llvm-project#176195 * [x] llvm/llvm-project#157073 * [x] llvm/llvm-project#176421 * [x] llvm/llvm-project#176925 * [x] llvm/llvm-project#177187
Update to LLVM 22 Scheduled release date: Feb 24 1.94 becomes stable: Mar 5 Changes: * Update to rc2, with one patch to work around our outdated illumos sysroot (rust-lang/llvm-project@41256ab). * Update the host toolchain as well, otherwise we lose cross-language LTO, in particular for jemalloc. * Adjust one loongarch assembly test. The split into r and s variants is based on the suggestion in rust-lang/rust#151134. Depends on: * [x] rust-lang/rust#151410 * [ ] rust-lang/rust#150756 * [x] llvm/llvm-project#175190 * [x] llvm/llvm-project#175912 * [x] llvm/llvm-project#175965 * [x] llvm/llvm-project#176195 * [x] llvm/llvm-project#157073 * [x] llvm/llvm-project#176421 * [x] llvm/llvm-project#176925 * [x] llvm/llvm-project#177187
Update to LLVM 22 Scheduled release date: Feb 24 1.94 becomes stable: Mar 5 Changes: * Update to rc2, with one patch to work around our outdated illumos sysroot (rust-lang/llvm-project@41256ab). * Update the host toolchain as well, otherwise we lose cross-language LTO, in particular for jemalloc. * Adjust one loongarch assembly test. The split into r and s variants is based on the suggestion in rust-lang/rust#151134. Depends on: * [x] rust-lang/rust#151410 * [ ] rust-lang/rust#150756 * [x] llvm/llvm-project#175190 * [x] llvm/llvm-project#175912 * [x] llvm/llvm-project#175965 * [x] llvm/llvm-project#176195 * [x] llvm/llvm-project#157073 * [x] llvm/llvm-project#176421 * [x] llvm/llvm-project#176925 * [x] llvm/llvm-project#177187
Update to LLVM 22 Scheduled release date: Feb 24 1.94 becomes stable: Mar 5 Changes: * Update to rc2, with one patch to work around our outdated illumos sysroot (rust-lang/llvm-project@41256ab). * Update the host toolchain as well, otherwise we lose cross-language LTO, in particular for jemalloc. * Adjust one loongarch assembly test. The split into r and s variants is based on the suggestion in rust-lang/rust#151134. Depends on: * [x] rust-lang/rust#151410 * [ ] rust-lang/rust#150756 * [x] llvm/llvm-project#175190 * [x] llvm/llvm-project#175912 * [x] llvm/llvm-project#175965 * [x] llvm/llvm-project#176195 * [x] llvm/llvm-project#157073 * [x] llvm/llvm-project#176421 * [x] llvm/llvm-project#176925 * [x] llvm/llvm-project#177187
Update to LLVM 22 Scheduled release date: Feb 24 1.94 becomes stable: Mar 5 Changes: * Update to rc2, with one patch to work around our outdated illumos sysroot (rust-lang/llvm-project@41256ab). * Update the host toolchain as well, otherwise we lose cross-language LTO, in particular for jemalloc. * Adjust one loongarch assembly test. The split into r and s variants is based on the suggestion in rust-lang/rust#151134. Depends on: * [x] rust-lang/rust#151410 * [ ] rust-lang/rust#150756 * [x] llvm/llvm-project#175190 * [x] llvm/llvm-project#175912 * [x] llvm/llvm-project#175965 * [x] llvm/llvm-project#176195 * [x] llvm/llvm-project#157073 * [x] llvm/llvm-project#176421 * [x] llvm/llvm-project#176925 * [x] llvm/llvm-project#177187
Currently running llvm-readobj --all on any MachO object asserts, because it implies --offload, and the --offload extraction includes an assert on the object format. Instead, we should be silently ignoring.
This regressed in #143342.